Move default value for --rpc-pubsub-notification-threads to CLI #158
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
steviez
commented
Mar 8, 2024
steviez
force-pushed
the
rpc_notif_threads
branch
2 times, most recently
from
3aac3a3 to
e71eeac
Compare
steviez
commented
Mar 9, 2024
| @@ -50,6 +50,7 @@ solana-net-utils = { workspace = true } | |||
| solana-perf = { workspace = true } | |||
| solana-poh = { workspace = true } | |||
| solana-program-runtime = { workspace = true } | |||
| solana-rayon-threadlimit = { workspace = true } | |||
There was a problem hiding this comment.
This crates goes away long-term, just bubbling this up so that this PR does not alter existing behavior
steviez
commented
Mar 9, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #158 +/- ##
========================================
Coverage 81.8% 81.8%
========================================
Files 838 838
Lines 226389 226525 +136
========================================
+ Hits 185342 185483 +141
+ Misses 41047 41042 -5 |
steviez
force-pushed
the
rpc_notif_threads
branch
2 times, most recently
from
dbf8de8 to
9d8c493
Compare
steviez
commented
Mar 12, 2024
Comment on lines
-1385
to
+1387
| notification_threads: if full_api { | ||
| value_of(&matches, "rpc_pubsub_notification_threads") | ||
| } else { | ||
| Some(0) | ||
| }, | ||
| notification_threads: value_t!(matches, "rpc_pubsub_notification_threads", usize) | ||
| .ok() | ||
| .and_then(NonZeroUsize::new), |
There was a problem hiding this comment.
Use of .requires() and .default_value_if() allow us to simplify this check. The scenarios are
full-rpc | n-thread | n-thread value
------------------------------------
false | false | value_t() will yield a None
false | true | CLAP errors out
true | false | value_t() will match the default
true | true | value_t() will match N (user value)
where:
full-rpc==>--full-rpc-apipassedn-thread==>--rpc-pubsub-notification-threads Npassedn-thread value==> The resulting value that is parsed and set inpubsub_config.notification_threads
The default value was previously being determined down where the thread pool is being created. Providing a default value at the CLI level is consistent with other args, and gives an operator better visibility into what the default will actually be
steviez
force-pushed
the
rpc_notif_threads
branch
from
6fc7b20 to
b9e7173
Compare
|
Had to force push in order to rebase and pull in some unrelated item that was making CI fail |
codebender828
pushed a commit
to codebender828/agave
that referenced
this pull request
Oct 3, 2024
…-xyz#158) The default value was previously being determined down where the thread pool is being created. Providing a default value at the CLI level is consistent with other args, and gives an operator better visibility into what the default will actually be
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The default value was previously being determined down where the thread pool is being created. Providing a default value at the CLI level is consistent with other args, and gives an operator better visibility into what the default will actually be.
Related to #105 - was doing something else and figured I'd clean this up
Things previously failed in CI when
--full-rpc-apibecame required because of the CLAP bug; I should get all greens on CI but I also manually started up a validator and did these sanity checks:--full-rpc-apiis not required--full-rpc-apican be used without--rpc-pubsub-notification-threads--rpc-pubsub-notification-threadserrors without--full-rpc-api--rpc-pubsub-notification-threadsproceeds with--full-rpc-api